Move and rename `SessionDiagnostic` & `SessionSubdiagnostic` traits and macros #101558

JhonnyBillM · 2022-09-08T04:25:51Z

After PR #101434, we want to:

Move SessionDiagnostic to rustc_errors.
Add emit_ methods that accept impl SessionDiagnostic to Handler.
(optional) Rename trait SessionDiagnostic to DiagnosticHandler.
(optional) Rename macro SessionDiagnostic to DiagnosticHandler.
Update Rustc Dev Guide and Docs to reflect these changes. [Diagnostics] - Update docs to match renamed Diagnostics traits and macros rustc-dev-guide#1460

Now I am having build issues getting the compiler to build when trying to rename the macro.

See diagnostics errors and context when building.

error: diagnostics should only be created in `SessionDiagnostic`/`AddSubdiagnostic` impls
  --> compiler/rustc_attr/src/session_diagnostics.rs:13:10
   |
13 |   #[derive(DiagnosticHandler)]
   |            ^^^^^^^^^^^^^^^^^ in this derive macro expansion
   |
  ::: /Users/jhonny/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/synstructure-0.12.6/src/macros.rs:94:9
   |
94 | /         pub fn $derives(
95 | |             i: $crate::macros::TokenStream
96 | |         ) -> $crate::macros::TokenStream {
   | |________________________________________- in this expansion of `#[derive(DiagnosticHandler)]`
   |
note: the lint level is defined here
  --> compiler/rustc_attr/src/lib.rs:10:9
   |
10 | #![deny(rustc::diagnostic_outside_of_impl)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And also this one:

error: diagnostics should only be created in `SessionDiagnostic`/`AddSubdiagnostic` impls
   --> compiler/rustc_attr/src/session_diagnostics.rs:213:32
    |
213 |         let mut diag = handler.struct_span_err_with_code(
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^^

Note
Can't find where this message is coming from, because you can see in this experimental branch that I updated all errors and diags to say:
error: diagnostics should only be created in DiagnosticHandler/AddSubdiagnostic impls
and not:
error: diagnostics should only be created in SessionDiagnostic/AddSubdiagnostic impls

I tried building the compiler in different ways (playing with the stages etc), but nothing worked.

Question

Do we need to build or do something different when renaming a macro and identifiers?

For context, see experimental commit JhonnyBillM@f2193a9 where the macro and symbols are renamed, but it doesn't compile.

rust-highfive · 2022-09-08T04:25:54Z

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

fee1-dead · 2022-09-08T08:17:36Z

r? @davidtwco

davidtwco

This is great, left some ideas for how to fix the errors you're seeing in a comment below. I think it might be good if we named the derive Diagnostic (i.e. #[derive(Diagnostic)]) rather than DiagnosticHandler. I don't mind if the trait is named DiagnosticHandler, though maybe something like IntoDiagnostic might be more appropriate.

Right now (without this PR), we have the following...

Derive	Trait
`#[derive(SessionDiagnostic)]`	`SessionDiagnostic`
`#[derive(SessionSubdiagnostic)]`	`AddToDiagnostic`
`#[derive(LintDiagnostic)]`	`DecorateLint`

But maybe it would be more uniform as..

Derive	Trait
`#[derive(Diagnostic)]`	`IntoDiagnostic`
`#[derive(Subdiagnostic)]`	`AddToDiagnostic`
`#[derive(LintDiagnostic)]`	`DecorateLint`

compiler/rustc_errors/src/diagnostic_builder.rs

compiler/rustc_errors/src/lib.rs

rustbot · 2022-09-09T16:18:25Z

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred in need_type_info.rs

cc @lcnr

davidtwco

LGTM, r=me (w/ rollup=never as this could easily conflict with other translation PRs) but I'd like it if you could squash some of your commits a little just so there are fewer that just correct previous commits in this PR.

JhonnyBillM · 2022-09-12T13:37:21Z

@davidtwco Updated commit history. Now it has 1 commit per rename, plus one with updates and fixes.

Thanks for the approval, I would like to confirm:

Are we ok with TargetDataLayoutErrorsWrapper in:
1.1 rustc_session/src/errors.rs:48
1.2 rustc_middle/src/ty/context.rs:1249
1.3 rustc_session/src/config.rs:902
Updated lint_diagnostic_derive in rustc_macros/src/diagnostics/mod.rs:106 to https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference. Is this ok?

davidtwco · 2022-09-12T14:04:52Z

@davidtwco Updated commit history. Now it has 1 commit per rename, plus one with updates and fixes.

Perfect!

Are we ok with TargetDataLayoutErrorsWrapper in:
1.1 rustc_session/src/errors.rs:48
1.2 rustc_middle/src/ty/context.rs:1249
1.3 rustc_session/src/config.rs:902

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

Updated lint_diagnostic_derive in rustc_macros/src/diagnostics/mod.rs:106 to https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference. Is this ok?

This is fine.

JhonnyBillM · 2022-09-12T14:17:04Z

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

We need it because of Coherence/Orphan rule. It wasn't needed before because the SessionDiagnostic protocol was defined in rustc_session, now that it isn't, neither the type nor the trait are defined in the crate.

But I am not sure if the compiler codebase handles this differently. Open to suggestions

davidtwco · 2022-09-12T14:30:17Z

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

We need it because of Coherence/Orphan rule. It wasn't needed before because the SessionDiagnostic protocol was defined in rustc_session, now that it isn't, neither the type nor the trait are defined in the crate.

But I am not sure if the compiler codebase handles this differently. Open to suggestions

I think rustc_errors depends on rustc_target for some IntoDiagnosticArg impls, so you might be able to move this impl there?

JhonnyBillM · 2022-09-12T15:02:12Z

@davidtwco Great point. Is it possible to move this error in a follow up PR?

Because:

right now rustc_errors doesn't have a fluent file.
we would have to move the fluent strings.
I think we should have a separate file for these "general IntoDiagnostic impls" in rustc_errors instead of having them all in rustc_errors/src/diagnostics.rs

this would introduce many changes unrelated to the move+rename of the traits, and another PR could be more appropriate (for example, when porting rustc_errors, or just in a general clean up task).

davidtwco · 2022-09-12T15:05:26Z

Sure, we can leave that to a follow-up.

@bors r+ rollup=never (likely to conflict with other translation pull requests)

rust-timer · 2022-09-22T00:18:12Z

Finished benchmarking commit (9062b78): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

…, r=davidtwco Clean up (sub)diagnostic derives The biggest chunk of this is unifying the parsing of subdiagnostic attributes (`#[error]`, `#[suggestion(...)]`, `#[label(...)]`, etc) between `Subdiagnostic` and `Diagnostic` type attributes as well as `Diagnostic` field attributes. It also improves a number of proc macro diagnostics. Waiting for rust-lang#101558.

…t-errors-wrapper, r=davidtwco Move `IntoDiagnostic` conformance for `TargetDataLayoutErrors` into `rustc_errors` Addressed this suggestion rust-lang#101558 (comment). This way we comply with the Coherence rule given that `IntoDiagnostic` trait is defined in `rustc_errors`, and almost all other crates depend on it.

rust-highfive assigned fee1-dead Sep 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2022

rust-highfive assigned davidtwco and unassigned fee1-dead Sep 8, 2022

davidtwco reviewed Sep 8, 2022

View reviewed changes

compiler/rustc_errors/src/diagnostic_builder.rs Outdated Show resolved Hide resolved

JhonnyBillM force-pushed the session-diagnostic-to-diagnostic-handler-refactor branch from c50ac27 to 2a860bd Compare September 9, 2022 06:05

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

davidtwco reviewed Sep 9, 2022

View reviewed changes

compiler/rustc_errors/src/lib.rs Outdated Show resolved Hide resolved

JhonnyBillM force-pushed the session-diagnostic-to-diagnostic-handler-refactor branch from 8157783 to 994434f Compare September 9, 2022 13:26

JhonnyBillM mentioned this pull request Sep 9, 2022

[Diagnostics] - Update docs to match renamed Diagnostics traits and macros rust-lang/rustc-dev-guide#1460

Merged

JhonnyBillM changed the title ~~[WIP] - Move and rename SessionDiagnostic to DiagnosticHandler~~ Move and rename SessionDiagnostic & SessionSubdiagnostic traits and macros Sep 9, 2022

JhonnyBillM marked this pull request as ready for review September 9, 2022 16:18

JhonnyBillM requested a review from davidtwco September 9, 2022 16:19

davidtwco approved these changes Sep 12, 2022

View reviewed changes

JhonnyBillM force-pushed the session-diagnostic-to-diagnostic-handler-refactor branch 3 times, most recently from ebbd3a2 to da3b080 Compare September 12, 2022 13:29

bors mentioned this pull request Sep 21, 2022

Migrating rustc_infer to session diagnostics (part 3) #101936

Merged

JhonnyBillM deleted the session-diagnostic-to-diagnostic-handler-refactor branch September 21, 2022 23:02

JhonnyBillM mentioned this pull request Sep 21, 2022

Diagnostic Translation #100717

Open

84 tasks

bors mentioned this pull request Sep 21, 2022

Migrate rustc_passes diagnostics #102110

Merged

22 tasks

This comment has been minimized.

Sign in to view

JhonnyBillM mentioned this pull request Oct 5, 2022

Move IntoDiagnostic conformance for TargetDataLayoutErrors into rustc_errors #102684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move and rename `SessionDiagnostic` & `SessionSubdiagnostic` traits and macros #101558

Move and rename `SessionDiagnostic` & `SessionSubdiagnostic` traits and macros #101558

JhonnyBillM commented Sep 8, 2022 •

edited

Loading

rust-highfive commented Sep 8, 2022

fee1-dead commented Sep 8, 2022

davidtwco left a comment

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Sep 9, 2022

davidtwco left a comment

JhonnyBillM commented Sep 12, 2022

davidtwco commented Sep 12, 2022

JhonnyBillM commented Sep 12, 2022 •

edited

Loading

davidtwco commented Sep 12, 2022

JhonnyBillM commented Sep 12, 2022

davidtwco commented Sep 12, 2022

rust-timer commented Sep 22, 2022

This comment has been minimized.

Move and rename SessionDiagnostic & SessionSubdiagnostic traits and macros #101558

Move and rename SessionDiagnostic & SessionSubdiagnostic traits and macros #101558

Conversation

JhonnyBillM commented Sep 8, 2022 • edited Loading

Question

rust-highfive commented Sep 8, 2022

fee1-dead commented Sep 8, 2022

davidtwco left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Sep 9, 2022

davidtwco left a comment

Choose a reason for hiding this comment

JhonnyBillM commented Sep 12, 2022

davidtwco commented Sep 12, 2022

JhonnyBillM commented Sep 12, 2022 • edited Loading

davidtwco commented Sep 12, 2022

JhonnyBillM commented Sep 12, 2022

davidtwco commented Sep 12, 2022

rust-timer commented Sep 22, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

This comment has been minimized.

Move and rename `SessionDiagnostic` & `SessionSubdiagnostic` traits and macros #101558

Move and rename `SessionDiagnostic` & `SessionSubdiagnostic` traits and macros #101558

JhonnyBillM commented Sep 8, 2022 •

edited

Loading

JhonnyBillM commented Sep 12, 2022 •

edited

Loading